Conversation
R/bundlePackageRenv.R
Outdated
| resolved <- renv::paths$lockfile(project = bundleDir) | ||
|
|
||
| # If the resolved file exists, and is different from the standard location, | ||
| # copy it to the standard location. |
There was a problem hiding this comment.
Why do we need to copy it at all? Why can't we use it where it lives? It's a little odd to me that this function would alter the contents of the current working directory.
There was a problem hiding this comment.
One thing that's confusing about this is that it's actually not copying it into the working directory, but actually into the temporary directory the rsconnect creates and copies files over to a temporary directory
Lines 130 to 135 in c704931
Lines 1 to 12 in c704931
But thinking about this more, we might need to make sure that we can handle relative paths in RENV_PATHS_LOCKFILE since when this is run inside of the temp bundle dir, those might not work (e.g. if RENV_PATHS_LOCKFILE="../renv.lock" and the app is at /home/jonkeane/Documents/myapp/subdir/, my lock file is at /home/jonkeane/Documents/myapp/renv.lock we don't want the target of this copy to be /tmp/.../renv.lock
R/bundlePackageRenv.R
Outdated
| if (file.exists(standard)) { | ||
| cli::cli_warn(c( | ||
| "Using lockfile at {.path {resolved}} instead of {.path {standard}}.", | ||
| i = "The lockfile in the project root may be outdated.", |
There was a problem hiding this comment.
If I run this function twice, I'm going to get this warning, even though the files themselves are identical. Should we be checking the contents of the files instead?
There was a problem hiding this comment.
I can add checking the contents, but running user-exported commands in rsconnect, this path should not alter someone's app directory (so this shouldn't happen in practice unless someone calls rsconnect:::ensureRenvLockFile (or the other internal functions that always operate on the temp bundle dir
|
Ok, this is ready for another look. Thanks for the comments, @nealrichardson those nudged me to take a different approach that I think is much cleaner (even if the copying wasn't as big of a deal as it seemed), since we don't actually need the lockfile in the bundle, now we more directly parse it in-situ. One small negative side effect of this is that we will not include a lock file that happens to be outside of the directory tree in the bundle even if it exists (and could in principle be added to the bundle). But that's a bit of a weird edgecase to support anyway. The TL;DR of this recent change is that I've made it so that |
|
Oh, and additionally some of the oddities around relative renv.lock file paths: rstudio/renv#2238 |
There was a problem hiding this comment.
I think we also need to make removeRenv aware of other potential lockfile locations
rsconnect/R/bundlePackageRenv.R
Line 177 in 591e652
| renv::snapshot(bundleDir, packages = deps$Package, prompt = FALSE) | ||
| # renv::snapshot() respects RENV_PATHS_LOCKFILE and renv profiles, so the | ||
| # lockfile may have been written to a non-standard location. | ||
| lockfile <- resolveRenvLockFile(bundleDir) |
There was a problem hiding this comment.
might be good to have some error handling here in case something goes wrong with resolving the lockfile, otherwise we're likely to get cryptic errors from parseRenvDependencies
| lockfile <- resolveRenvLockFile(bundleDir) | |
| lockfile <- resolveRenvLockFile(bundleDir) | |
| if (is.null(lockfile)) { | |
| cli::cli_abort("renv::snapshot() did not produce a lockfile") | |
| } |
3 participants
Support renv profiles and custom renv file locations specified with the
RENV_PATHS_LOCKFILEenvironment variableFixes #1122